Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix errors on 1.7 #102

Merged
merged 5 commits into from
Feb 12, 2022
Merged

Conversation

darsnack
Copy link
Contributor

@darsnack darsnack commented Jul 28, 2021

DataType no longer has a mutable field. The new function ismutabletype can be used instead. Similarly, abstract is now isabstracttype.

jl_new_datatype now takes in a fattrs argument which I think should be T.atomicfields (maybe @vtjnash can correct me). The change was added in JuliaLang/julia#37847

Only remaining error is that accessing the field N for Vararg{T} is now an undefined reference.

Fixes #107

@darsnack
Copy link
Contributor Author

Okay all the errors should have been resolved @DhairyaLGandhi

src/extensions.jl Outdated Show resolved Hide resolved
@darsnack
Copy link
Contributor Author

It seems we were only running into that issue for Vararg{T}, so I added a specific path for just that case and reverted the isdefined change. I also added some more tests for undefined references. There is one case that is currently broken (i.e. #43).

@vtjnash can you comment on what the fattrs argument to jl_new_datatype is supposed to represent? Seems like field attributes (e.g. atomic), but I wasn't sure what I should be passing in.

@vtjnash
Copy link

vtjnash commented Jul 29, 2021

It is unlikely that you should be trying to access internal functions via ccall. The allocator for struct objects is an intrinsic function: Core._structtype(Main, :A, %3, %4, %5, false, 0) (see @Meta.lower struct A; end), though the other functions called there are also mandatory, so calling eval with a struct definition is perhaps the most reliable version.

@darsnack
Copy link
Contributor Author

@CarloLucibello I don't have commit rights here, can you review?

The fixes here touch internals, but it is no more hacky than the rest of the package in that regard. A full overhaul to make the package more stable in the long term is needed (we already see failures on nightly), but not a task that I can complete in the near future. I would suggest this is merged so that BSON.jl is usable on 1.7.

@CarloLucibello
Copy link
Collaborator

old tests are passing, new tests are added, good enough for me

@CarloLucibello CarloLucibello merged commit 379fb3c into JuliaIO:master Feb 12, 2022
@darsnack darsnack deleted the darsnack/mutable-fix branch February 12, 2022 15:56
@darsnack
Copy link
Contributor Author

We should tag a patch release too

@CarloLucibello
Copy link
Collaborator

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem saving function in BSON
4 participants